feat(observability): add observability crate and unify process init - #171
Open
Bnjoroge1 wants to merge 2 commits into
Open
feat(observability): add observability crate and unify process init#171Bnjoroge1 wants to merge 2 commits into
Bnjoroge1 wants to merge 2 commits into
Conversation
Create crates/preloop-observability with the small explicit API from Plan 002: ObservabilityConfig::from_env (PRELOOP_LOG_FORMAT auto/pretty/json, RUST_LOG default info, OTEL_SERVICE_NAME, OTEL_EXPORTER_OTLP_* with redacted Debug), Observability::noop (no socket), Observability::from_config + ObservabilityRuntime (2s bounded shutdown), TaskHeartbeat registry (critical vs non-critical, Drop deregisters, any_critical_stale) and LimitRegistry (register/record_drop/record_reject with &'static str keys). No SDK, no OTLP http-proto, no Prometheus reader yet — host-only features are deferred; the handle is cloneable into AppState and RunnerPoolConfig and makes tests perform no network I/O. Wire the same init into all three binaries: - preloop-cli/src/main.rs: replace fmt::init with Observability::from_config + install_fmt_subscriber, hold handle for later pool wiring - preloop-runner-server/src/main.rs: same, fixing the missing info fallback (from_default_env with no default hid pool faults) - preloop-runner/src/main.rs: structured local logger only, never export PRELOOP_LOG_FORMAT auto now means pretty on TTY and JSON when piped (no ANSI in journald), via IsTerminal. Verify: cargo test -p preloop-observability 9 passed (noop, absent endpoint disabled not localhost, none disables, heartbeat deregister, critical staleness, limit counts, Debug redaction, shutdown bounded); cargo check --locked --workspace and cargo check -p preloop-cli -p preloop-runner-server -p preloop-runner all pass; just sg-scan-strict and cargo fmt --all --check pass. Entire-Checkpoint: 01M0GC36ZYTA6J8Q1E0HY264R1
anyhow is unused in the crate, and serde_json/tokio/tracing were declared twice under dev-dependencies (the workspace tokio features already cover sync/time/rt). Entire-Checkpoint: 01M0GZ2NAT9NC1F5A0RS5XKKTX
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New
preloop-observabilitycrate: env-driven logging (RUST_LOGdefaults toinfo;PRELOOP_LOG_FORMATpretty/json/auto), task heartbeats with criticality, limit registry, VM telemetry registry, and theObservabilityhandle that later PRs clone into the server and pool. All three binaries install the unified subscriber sopreloop serveand the CLI stop disagreeing about log level. AbsentOTEL_*env means disabled — no defaultlocalhost:4318socket.Part of a stacked series (merge bottom-up):
Summary by cubic
Introduces
preloop-observabilityand unifies process-level logging init acrosspreloop,preloop-runner-server, andpreloop-runner. The server’s default log level changes from “silent unless RUST_LOG is set” to info, matching the CLI;PRELOOP_LOG_FORMATnow controls pretty/json/auto output, and OTLP export stays disabled unless an OTEL endpoint is provided (no implicit localhost:4318).preloop-observability:ObservabilityConfig::from_env(RUST_LOG default info; PRELOOP_LOG_FORMAT auto/pretty/json; OTEL vars parsed; Debug redacts secrets).Observabilityhandle andObservabilityRuntimewith a bounded 2s shutdown window.tracing_subscribersetup in all binaries with one subscriber install viaObservabilityRuntime::install_fmt_subscriber; the handle is kept for later wiring into server/pool.Written for commit 454c45e. Summary will update on new commits.
Note
Add
preloop-observabilitycrate and unify logging init across CLI, runner, and runner-serverpreloop-observabilitywithObservabilityConfig,Observability, andObservabilityRuntimetypes. Config is built from env:RUST_LOGdefaults toinfo,PRELOOP_LOG_FORMATselectspretty/json/auto(auto maps to pretty on TTY, json otherwise).TaskHeartbeatregistry for tracking long-lived background tasks with staleness detection on critical tasks, andLimitRegistryfor recording dropped/rejected event counts per named limit.ObservabilityRuntime::install_fmt_subscriberinstalls a global tracing fmt subscriber honoring the resolved log format and filter.shutdown()is bounded to 2 seconds (currently a no-op placeholder).preloop-cli,preloop-runner, andpreloop-runner-serverreplace directtracing_subscriberinit with the new crate. All three now default toinfowhenRUST_LOGis unset.ObservabilityConfigcustomDebugimpl redactsOTLPheaders and endpoint secrets; if redaction logic is incomplete, sensitive values could leak in logs. The global subscriber is installed once per process, so concurrent or repeatedinstall_fmt_subscribercalls will panic.📊 Macroscope summarized 454c45e. 9 files reviewed, 7 issues evaluated, 7 issues filtered, 0 comments posted
🗂️ Filtered Issues
crates/preloop-observability/src/lib.rs — 0 comments posted, 7 evaluated, 7 filtered
from_envapplies the empty/nonefilter only after the.or_else(...)chain. Therefore, ifOTEL_EXPORTER_OTLP_ENDPOINTexists but is empty/whitespace, it prevents a valid signal-specific endpoint such asOTEL_EXPORTER_OTLP_TRACES_ENDPOINTfrom being considered, andotlp_enabledbecomes false even though an endpoint is present. Filter each candidate before selecting the fallback (while preserving the intentional globalnoneoverride if desired). [ Out of scope (post-validation triage) ]OTEL_EXPORTER_OTLP_HEADERSvalue wins the.or_else(...)chain and is then removed, so a non-empty signal-specific headers variable is ignored. Consequentlyhas_otel_headers()incorrectly reports false despite usable OTLP headers being configured. [ Out of scope (post-validation triage) ]HeartbeatHandle, whose unconditionalDropremoves the map entry. When a spawned task panics, Rust unwinds and drops the handle too, so the dead critical task disappears fromTaskHeartbeat;any_critical_stalethen cannot detect it and/readyzcan remain healthy despite loss of a critical background task. The registry must distinguish clean completion from panic (or retain a failed/stale entry). [ Out of scope ]TaskHeartbeatstores oneHeartbeatEntryper task name, butregisterreturns independent guards and silently replaces an existing same-name entry. If two overlapping tasks register the same static name, dropping the older guard removes the newer task's entry; subsequent beats become no-ops and readiness can incorrectly report healthy because the critical task is no longer registered. The map needs per-registration identity/ref-counting, or duplicate registration must be rejected. [ Out of scope (post-validation triage) ]registersilently replaces an existing entry with the samename, but both returned handles remain able to deregister by name. If a replacement/restarted task is registered before the old handle drops, dropping the old handle removes the replacement's entry; subsequent beats become no-ops and readiness/status stop monitoring the still-running task. The handle needs a per-registration identity (or duplicate registration must be rejected) soDroponly removes its own entry. [ Out of scope (post-validation triage) ]HeartbeatHandle::dropalways deregisters the task, including when the task is being unwound after a panic (Rust runs destructors during panic unwinding). A panicked critical background task therefore disappears fromsnapshotandany_critical_stale, so/readyzcan continue reporting healthy after the critical task has died. Checkstd::thread::panicking()and preserve/mark the entry on panic rather than removing it. [ Out of scope (post-validation triage) ]absent_endpoint_means_disabled_not_localhostmutates process-global OTEL environment variables without synchronizing withnone_disables_signalanddebug_redacts_headers_and_endpoint_userinfo. Rust tests run concurrently by default, so either test can set an endpoint between these removals andObservabilityConfig::from_env(), making this test flaky (and its removals can likewise break the other tests). Serialize all environment-mutating tests and restore prior values. [ Out of scope (post-validation triage) ]